-
-
Notifications
You must be signed in to change notification settings - Fork 362
feat(Html2Pdf): add PaperFormat parameter #7036
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR enhances the Html2Pdf feature by adding a configurable PaperFormat parameter along with related printing options (background, margins, scale, header/footer), implemented via new option classes, updated sample usage, and corresponding unit tests. Sequence diagram for updated PDF export process with PaperFormatsequenceDiagram
actor User
participant Html2PdfService
participant DownloadService
User->>Html2PdfService: PdfStreamFromHtmlAsync(htmlString, cssFiles, options: PdfOptions)
Html2PdfService->>Html2PdfService: Generate PDF with options (Format, PrintBackground, etc.)
Html2PdfService-->>User: PDF stream
User->>DownloadService: DownloadFromStreamAsync(filename, stream)
DownloadService-->>User: PDF file downloaded
Class diagram for new and updated PDF option typesclassDiagram
class PdfOptions {
bool Landscape
bool PrintBackground
PaperFormat Format
bool DisplayHeaderFooter
decimal Scale
MarginOptions MarginOptions
}
class PaperFormat {
decimal Width
decimal Height
PaperFormat(string width, string height)
<<static>> PaperFormat Letter
<<static>> PaperFormat Legal
<<static>> PaperFormat Tabloid
<<static>> PaperFormat Ledger
<<static>> PaperFormat A0
<<static>> PaperFormat A1
<<static>> PaperFormat A2
<<static>> PaperFormat A3
<<static>> PaperFormat A4
<<static>> PaperFormat A5
<<static>> PaperFormat A6
}
class MarginOptions {
object? Top
object? Left
object? Bottom
object? Right
}
PdfOptions --> PaperFormat : Format
PdfOptions --> MarginOptions : MarginOptions
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Add a using directive for System.Diagnostics.CodeAnalysis (or fully qualify) so that the ExcludeFromCodeCoverage attributes compile correctly.
- Consider changing MarginOptions properties from object? to a more specific type (e.g., string) to avoid boxing and improve type safety.
- Add unit tests for the PaperFormat static members to verify their predefined width/height values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add a using directive for System.Diagnostics.CodeAnalysis (or fully qualify) so that the ExcludeFromCodeCoverage attributes compile correctly.
- Consider changing MarginOptions properties from object? to a more specific type (e.g., string) to avoid boxing and improve type safety.
- Add unit tests for the PaperFormat static members to verify their predefined width/height values.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Options/PaperFormat.cs:12` </location>
<code_context>
+/// 纸张规格配置类
+/// </summary>
+[ExcludeFromCodeCoverage]
+public record PaperFormat
+{
+ /// <summary>
</code_context>
<issue_to_address>
**suggestion:** Consider making PaperFormat immutable for thread safety and consistency.
Width and Height should be init-only or readonly to prevent changes after creation, ensuring immutability for this value object.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Options/MarginOptions.cs:14-32` </location>
<code_context>
+ /// <summary>
+ /// Top margin, accepts values labeled with units.
+ /// </summary>
+ public object? Top { get; set; }
+
+ /// <summary>
+ /// Left margin, accepts values labeled with units.
+ /// </summary>
+ public object? Left { get; set; }
+
+ /// <summary>
+ /// Bottom margin, accepts values labeled with units.
+ /// </summary>
+ public object? Bottom { get; set; }
+
+ /// <summary>
+ /// Right margin, accepts values labeled with units.
+ /// </summary>
+ public object? Right { get; set; }
+}
</code_context>
<issue_to_address>
**suggestion:** Using object? for margin properties may reduce type safety.
Consider specifying a more precise type for margin properties to prevent runtime issues and make the code clearer.
```suggestion
/// <summary>
/// Top margin, accepts values labeled with units (e.g., "10px", "2em", "5%").
/// </summary>
public string? Top { get; set; }
/// <summary>
/// Left margin, accepts values labeled with units (e.g., "10px", "2em", "5%").
/// </summary>
public string? Left { get; set; }
/// <summary>
/// Bottom margin, accepts values labeled with units (e.g., "10px", "2em", "5%").
/// </summary>
public string? Bottom { get; set; }
/// <summary>
/// Right margin, accepts values labeled with units (e.g., "10px", "2em", "5%").
/// </summary>
public string? Right { get; set; }
```
</issue_to_address>
### Comment 3
<location> `test/UnitTest/Options/PdfOptionsTest.cs:10-19` </location>
<code_context>
+
+public class PdfOptionsTest
+{
+ [Fact]
+ public void Test_PdfOptions_Ok()
+ {
+ var options = new PdfOptions()
+ {
+ Scale = 1.0m,
+ DisplayHeaderFooter = true,
+ PrintBackground = true,
+ Landscape = true,
+ Format = PaperFormat.A1,
+ MarginOptions = new MarginOptions()
+ {
+ Top = "10mm",
+ Bottom = "10mm",
+ Left = "10mm",
+ Right = "10mm"
+ }
+ };
+ Assert.Equal(1.0m, options.Scale);
</code_context>
<issue_to_address>
**suggestion (testing):** Missing tests for default values and edge cases in PdfOptions.
Please add tests for default property values and edge cases, such as boundary values for Scale and handling of MarginOptions when unset or null.
Suggested implementation:
```csharp
public class PdfOptionsTest
{
[Fact]
public void Test_PdfOptions_Ok()
{
var options = new PdfOptions()
{
Scale = 1.0m,
DisplayHeaderFooter = true,
PrintBackground = true,
Landscape = true,
Format = PaperFormat.A1,
MarginOptions = new MarginOptions()
{
Top = "10mm",
Bottom = "10mm",
Left = "10mm",
Right = "10mm"
}
};
Assert.Equal(1.0m, options.Scale);
Assert.True(options.DisplayHeaderFooter);
Assert.True(options.PrintBackground);
Assert.True(options.Landscape);
Assert.Equal(PaperFormat.A1, options.Format);
Assert.Equal("10mm", options.MarginOptions.Top);
Assert.Equal("10mm", options.MarginOptions.Bottom);
Assert.Equal("10mm", options.MarginOptions.Left);
Assert.Equal("10mm", options.MarginOptions.Right);
}
[Fact]
public void Test_PdfOptions_DefaultValues()
{
var options = new PdfOptions();
Assert.Equal(1.0m, options.Scale); // Assuming default is 1.0m
Assert.False(options.DisplayHeaderFooter); // Assuming default is false
Assert.False(options.PrintBackground); // Assuming default is false
Assert.False(options.Landscape); // Assuming default is false
Assert.Equal(PaperFormat.A4, options.Format); // Assuming default is A4
Assert.NotNull(options.MarginOptions);
Assert.Equal("0mm", options.MarginOptions.Top); // Assuming default is "0mm"
Assert.Equal("0mm", options.MarginOptions.Bottom);
Assert.Equal("0mm", options.MarginOptions.Left);
Assert.Equal("0mm", options.MarginOptions.Right);
}
[Theory]
[InlineData(0.1)]
[InlineData(2.0)]
public void Test_PdfOptions_ScaleBoundary(decimal scale)
{
var options = new PdfOptions() { Scale = scale };
Assert.Equal(scale, options.Scale);
}
[Fact]
public void Test_PdfOptions_MarginOptions_Null()
{
var options = new PdfOptions() { MarginOptions = null };
Assert.Null(options.MarginOptions);
}
}
```
- If the actual default values for `PdfOptions` or `MarginOptions` differ from those assumed (e.g., Scale, Format, margin strings), adjust the assertions accordingly.
- If `MarginOptions` is not instantiated by default in `PdfOptions`, update the default value test to expect `null` instead of `NotNull`.
- If `PdfOptions` or `MarginOptions` have validation logic for boundary values, consider adding tests for invalid values and expected exceptions.
</issue_to_address>
### Comment 4
<location> `src/BootstrapBlazor/Options/PaperFormat.cs:17` </location>
<code_context>
+ /// <summary>
+ ///
+ /// </summary>
+ public static PaperFormat Letter => new(8.5m, 11m);
+
+ /// <summary>
</code_context>
<issue_to_address>
**issue (complexity):** Consider using a centralized mapping for paper formats to avoid repetitive instantiation in each static property.
Consider centralizing the size-to-dimension mapping (once) instead of repeating `new(…,…)` in every static prop. For example, define an enum and either a switch factory or a single Dictionary:
```csharp
public enum StandardPaperFormat
{
Letter, Legal, Tabloid, Ledger,
A0, A1, A2, A3, A4, A5, A6
}
public record PaperFormat(decimal Width, decimal Height)
{
private static readonly IReadOnlyDictionary<StandardPaperFormat, PaperFormat> _map
= new Dictionary<StandardPaperFormat, PaperFormat>
{
[StandardPaperFormat.Letter] = new(8.5m, 11m),
[StandardPaperFormat.Legal] = new(8.5m, 14m),
[StandardPaperFormat.Tabloid] = new(11m, 17m),
[StandardPaperFormat.Ledger] = new(17m, 11m),
[StandardPaperFormat.A0] = new(33.1102m, 46.811m),
[StandardPaperFormat.A1] = new(23.3858m, 33.1102m),
// …
};
public static PaperFormat Create(StandardPaperFormat format)
=> _map.TryGetValue(format, out var pf)
? pf
: throw new ArgumentOutOfRangeException(nameof(format), format, null);
}
```
If you still want named properties for discoverability, point them at the dictionary:
```csharp
public static PaperFormat Letter => Create(StandardPaperFormat.Letter);
public static PaperFormat Legal => Create(StandardPaperFormat.Legal);
// …
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /// 纸张规格配置类 | ||
| /// </summary> | ||
| [ExcludeFromCodeCoverage] | ||
| public record PaperFormat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider making PaperFormat immutable for thread safety and consistency.
Width and Height should be init-only or readonly to prevent changes after creation, ensuring immutability for this value object.
| /// <summary> | ||
| /// Top margin, accepts values labeled with units. | ||
| /// </summary> | ||
| public object? Top { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Left margin, accepts values labeled with units. | ||
| /// </summary> | ||
| public object? Left { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Bottom margin, accepts values labeled with units. | ||
| /// </summary> | ||
| public object? Bottom { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Right margin, accepts values labeled with units. | ||
| /// </summary> | ||
| public object? Right { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Using object? for margin properties may reduce type safety.
Consider specifying a more precise type for margin properties to prevent runtime issues and make the code clearer.
| /// <summary> | |
| /// Top margin, accepts values labeled with units. | |
| /// </summary> | |
| public object? Top { get; set; } | |
| /// <summary> | |
| /// Left margin, accepts values labeled with units. | |
| /// </summary> | |
| public object? Left { get; set; } | |
| /// <summary> | |
| /// Bottom margin, accepts values labeled with units. | |
| /// </summary> | |
| public object? Bottom { get; set; } | |
| /// <summary> | |
| /// Right margin, accepts values labeled with units. | |
| /// </summary> | |
| public object? Right { get; set; } | |
| /// <summary> | |
| /// Top margin, accepts values labeled with units (e.g., "10px", "2em", "5%"). | |
| /// </summary> | |
| public string? Top { get; set; } | |
| /// <summary> | |
| /// Left margin, accepts values labeled with units (e.g., "10px", "2em", "5%"). | |
| /// </summary> | |
| public string? Left { get; set; } | |
| /// <summary> | |
| /// Bottom margin, accepts values labeled with units (e.g., "10px", "2em", "5%"). | |
| /// </summary> | |
| public string? Bottom { get; set; } | |
| /// <summary> | |
| /// Right margin, accepts values labeled with units (e.g., "10px", "2em", "5%"). | |
| /// </summary> | |
| public string? Right { get; set; } |
| /// <summary> | ||
| /// | ||
| /// </summary> | ||
| public static PaperFormat Letter => new(8.5m, 11m); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider using a centralized mapping for paper formats to avoid repetitive instantiation in each static property.
Consider centralizing the size-to-dimension mapping (once) instead of repeating new(…,…) in every static prop. For example, define an enum and either a switch factory or a single Dictionary:
public enum StandardPaperFormat
{
Letter, Legal, Tabloid, Ledger,
A0, A1, A2, A3, A4, A5, A6
}
public record PaperFormat(decimal Width, decimal Height)
{
private static readonly IReadOnlyDictionary<StandardPaperFormat, PaperFormat> _map
= new Dictionary<StandardPaperFormat, PaperFormat>
{
[StandardPaperFormat.Letter] = new(8.5m, 11m),
[StandardPaperFormat.Legal] = new(8.5m, 14m),
[StandardPaperFormat.Tabloid] = new(11m, 17m),
[StandardPaperFormat.Ledger] = new(17m, 11m),
[StandardPaperFormat.A0] = new(33.1102m, 46.811m),
[StandardPaperFormat.A1] = new(23.3858m, 33.1102m),
// …
};
public static PaperFormat Create(StandardPaperFormat format)
=> _map.TryGetValue(format, out var pf)
? pf
: throw new ArgumentOutOfRangeException(nameof(format), format, null);
}If you still want named properties for discoverability, point them at the dictionary:
public static PaperFormat Letter => Create(StandardPaperFormat.Letter);
public static PaperFormat Legal => Create(StandardPaperFormat.Legal);
// …
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7036 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 743 743
Lines 32470 32475 +5
Branches 4500 4500
=========================================
+ Hits 32470 32475 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends the PdfOptions class with new properties to provide more granular control over PDF generation, including paper format, scaling, margins, and display settings.
- Extended
PdfOptionswith 5 new properties:PrintBackground,Format,DisplayHeaderFooter,Scale, andMarginOptions - Added two new supporting classes:
PaperFormat(with standard paper size presets) andMarginOptions(for page margin configuration) - Updated project versions and dependencies to reflect the changes
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/BootstrapBlazor/Options/PdfOptions.cs |
Added new PDF configuration properties with default values |
src/BootstrapBlazor/Options/PaperFormat.cs |
Created record class with predefined paper format constants (Letter, Legal, A0-A6, etc.) |
src/BootstrapBlazor/Options/MarginOptions.cs |
Created record class for configuring page margins |
test/UnitTest/Options/PdfOptionsTest.cs |
Added unit test validating the new PDF options functionality |
src/BootstrapBlazor/BootstrapBlazor.csproj |
Bumped version numbers for both Visual Studio 17.0 and 18.0 |
src/BootstrapBlazor.Server/BootstrapBlazor.Server.csproj |
Updated Html2Pdf package dependency version |
src/BootstrapBlazor.Server/Components/Samples/Html2Pdfs.razor.cs |
Updated sample to demonstrate new PdfOptions usage |
Comments suppressed due to low confidence (1)
src/BootstrapBlazor/Options/PaperFormat.cs:1
- All XML documentation comments in this class are empty. The summary tags should describe what each property, parameter, and static paper format represents (e.g., "获得 Letter 纸张格式 (8.5 x 11 英寸)" for Letter, "获得/设置 纸张宽度(英寸)" for Width property, etc.).
// Licensed to the .NET Foundation under one or more agreements.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <summary> | ||
| /// 获得/设置 放大比例 默认 1 取值 0.1 到 2 之间 | ||
| /// </summary> | ||
| public decimal Scale { get; set; } = 1; |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states the valid range is 0.1 to 2, but there is no validation enforcing this constraint. Consider adding validation in a setter or validation method to ensure the Scale value remains within the documented range.
| public decimal Scale { get; set; } = 1; | |
| private decimal _scale = 1; | |
| /// <summary> | |
| /// 获得/设置 放大比例 默认 1 取值 0.1 到 2 之间 | |
| /// </summary> | |
| public decimal Scale | |
| { | |
| get => _scale; | |
| set | |
| { | |
| if (value < 0.1m || value > 2m) | |
| { | |
| throw new ArgumentOutOfRangeException(nameof(Scale), "Scale must be between 0.1 and 2."); | |
| } | |
| _scale = value; | |
| } | |
| } |
Link issues
fixes #7035
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enable configurable paper format and related layout options for PDF generation by introducing PaperFormat and MarginOptions types, extending PdfOptions, and updating samples and tests accordingly.
New Features:
Enhancements:
Tests: